Fix race condition with --coverage and babel-jest identical file contents edge case #4432

stipsan · 2017-09-05T22:49:39Z

Summary

There's a super edge case race condition in the way babel-jest generates its cache key that affects code coverage reports on files that are identical except for their path.
It only affects cases where you have the same file contents in separate files that have different dirnames. This is not uncommon in large codebases though where some use cases encourage code duplication over KISS for various reasons. However when it does happen it can be very frustrating as most devs won't even know where to start trying to figure out why some coverage tests suddenly start failing.
There's at least one reported case related to this: #3968
There may be more, I'll have another go at finding them after this PR is merged or closed.
I started with creating a repo that shows how to reproduce the problem: https://github.com/stipsan/jest-identical-files-coverage-bug
But I decided to attempt fixing the problem, and do a bug report if I failed to find and resolve the issue in jest itself.

Test plan

The fix itself is very simple, just make the filename part of the cache key (which jest-runtime is doing), writing a test though is super hard because it does not happen if you run tests with --no-cache (which most integration tests in jest is doing, to avoid false passing tests, if my understanding is correct).
But a wise coder once said that if it's a super edge case then testing it is super important.
The attached test makes sure to cover the edge case by running the relevant test twice, once to prime the cache and the second time to test if it reports coverage correctly.
In my testing I noticed several cases where the race condition would not occur if the cache directory were empty. But if it primes the cache first it consistently fails on the second test run.

I tried my best to follow existing testing conventions and live up to the coding standard of the project codebase. Since I am new to the codebase however I am eager to adjust the PR to your feedback.

cpojer · 2017-09-05T22:53:24Z

packages/babel-jest/src/index.js

@@ -90,6 +90,8 @@ const createTransformer = (options: any) => {
        .update('\0', 'utf8')
        .update(fileData)
        .update('\0', 'utf8')
+        .update(filename)


Can we make this relative to the rootDir (if it isn't already)?

It's not, checking 😄

In order to make it relative to rootDir we'll have to pass it here first:
https://github.com/facebook/jest/blob/0cb03f6474f52964ed74eadcde2ae3e219b4d04e/packages/jest-runtime/src/script_transformer.js#L77-L80

Fine with me.

Alright I'm on it.

Pushed it, I also changed a flowtype hint as it appeared to be incorrect: 69456ca#diff-066bbe3aae5baf14d3b62017f0dc86d4R85

According to the definition here: https://github.com/facebook/jest/blob/0cb03f6474f52964ed74eadcde2ae3e219b4d04e/types/Transform.js#L37-L41

getCacheKey gets CacheKeyOptions not TransformOptions .

stipsan · 2017-09-05T23:31:50Z

@cpojer pushed, looks like CIs are happy 😃

codecov-io · 2017-09-06T01:42:21Z

Codecov Report

Merging #4432 into master will increase coverage by 0.26%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4432      +/-   ##
==========================================
+ Coverage   56.66%   56.92%   +0.26%     
==========================================
  Files         191      189       -2     
  Lines        6496     6461      -35     
  Branches        6        3       -3     
==========================================
- Hits         3681     3678       -3     
+ Misses       2812     2782      -30     
+ Partials        3        1       -2

Impacted Files	Coverage Δ
packages/jest-runtime/src/script_transformer.js	`89.92% <ø> (ø)`	⬆️
...s/coverage_report/cached-duplicates/b/identical.js	`100% <100%> (ø)`
...s/coverage_report/cached-duplicates/a/identical.js	`100% <100%> (ø)`
packages/jest-message-util/src/index.js	`86.41% <0%> (-0.17%)`	⬇️
packages/jest-haste-map/src/crawlers/node.js	`95.65% <0%> (-0.07%)`	⬇️
...ckages/jest-cli/src/reporters/coverage_reporter.js	`54.9% <0%> (ø)`	⬆️
packages/jest-cli/src/run_jest.js	`0% <0%> (ø)`	⬆️
...ackages/pretty-format/src/plugins/react_element.js	`100% <0%> (ø)`	⬆️
...ages/jest-config/src/reporter_validation_errors.js	`16.66% <0%> (ø)`	⬆️
packages/jest-cli/src/get_no_test_found.js	`0% <0%> (ø)`	⬆️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2c910dc...6893d71. Read the comment docs.

cpojer · 2017-09-06T09:43:35Z

Very much appreciate the thorough summary, added tests and the fix. Thank you so much for your contribution to Jest, this is awesome.

cpojer · 2017-09-06T09:43:53Z

cc @aaronabramov check this PR out!

stipsan · 2017-09-06T09:56:54Z

Thanks @cpojer! I really appreciate the time all the maintainers and contributors spend making jest great. To me that makes it a no-brainer to spend extra time making the PR easier to review 😄

…ents edge case (jestjs#4432) * Add test case * Fix cache race condition by including filename in cache key * make filename cache key relative to rootDir * Update snapshot

gdmitchell · 2018-02-10T00:06:32Z

I'm still having this problem with 21.2.1, 22.2.0, and 22.2.2 (I'm getting 0 coverage reported for all but one of the files that are identical except for their path) unless I run with --no-cache, which is less than ideal.

If I have two files that are identical except for their paths, it's more or less random which one will show the proper coverage and which one will show 0.

stipsan · 2018-02-16T16:08:51Z

@gdmitchell can you make a repo to reproduce this? It sounds like you're having a similar problem, but it if it's a regression directly related to this PR then I would've experienced it in my own projects as well as you. But in my case it's been reliable for months now 😮

gdmitchell · 2018-02-16T17:34:32Z

@stipsan Unfortunately, I haven't been able to repro it on anything but a work repo that I can't share. I tried making a minimal repo of just the files that exhibited the problem but when I did so, clearing the cache made the problem go away, which it doesn't in the original repo. 🤔

stipsan · 2018-02-16T20:25:45Z

I understand. An alternative could be that you redact any sensitive information from your jest config file and share that and I might be able to help you see if we can reproduce it somehow 🙂

Do you use any setup like lerna, yarn workspaces or NODE_PATH or similar btw?

gdmitchell · 2018-02-16T22:15:21Z

Here's my package.json with a few redactions, which includes my Jest config:
package_json.txt

Note that in scripts I would normally have just the Jest command instead of jest --no-cache, but I've added --no-cache for the time being as a workaround for this problem.

We do use Yarn, though in my testing I've also been running Jest directly and it doesn't seem to make any difference.

stipsan · 2018-02-17T00:30:21Z

Ok thanks! This is interesting <rootDir>/jest/preprocessor.js, are you able to share that as well?

Transformers are affected by wether you run with --no-cache or not 🤔

Btw I meant the yarn monorepo feature they call workspaces: https://yarnpkg.com/lang/en/docs/workspaces/
Yarn by itself makes no difference 😄

gdmitchell · 2018-02-19T18:07:34Z

Ahh, yeah. We don't use workspaces as far as I'm aware.

Here's preprocessor.js. Just changed the names of a couple variables but the idea is that it does babel.transform on anything not in node_modules or that is one of our own dependencies (library1/library2).
preprocessor_js.txt

github-actions · 2021-05-12T20:06:58Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

stipsan added 2 commits September 6, 2017 00:06

Add test case

4f18758

Fix cache race condition by including filename in cache key

e9afe00

facebook-github-bot added the CLA Signed ✔️ label Sep 5, 2017

cpojer reviewed Sep 5, 2017

View reviewed changes

stipsan added 2 commits September 6, 2017 01:11

make filename cache key relative to rootDir

69456ca

Update snapshot

6893d71

cpojer merged commit 16431b5 into jestjs:master Sep 6, 2017

stipsan deleted the bugfix/cached-coverage-duplicates branch September 6, 2017 09:57

This was referenced Sep 6, 2017

Jest coverage targets the wrong file #3968

Closed

Jest coverage reporting #3753

Closed

Moving src files doesn't bust the Jest cache #3705

Closed

thymikee mentioned this pull request Sep 11, 2017

cache write race condition across processes #4444

Open

github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix race condition with --coverage and babel-jest identical file contents edge case #4432

Fix race condition with --coverage and babel-jest identical file contents edge case #4432

stipsan commented Sep 5, 2017

cpojer Sep 5, 2017

stipsan Sep 5, 2017

stipsan Sep 5, 2017

cpojer Sep 5, 2017

stipsan Sep 5, 2017 •

edited

Loading

stipsan Sep 5, 2017

stipsan commented Sep 5, 2017

codecov-io commented Sep 6, 2017 •

edited

Loading

cpojer commented Sep 6, 2017

cpojer commented Sep 6, 2017

stipsan commented Sep 6, 2017

gdmitchell commented Feb 10, 2018 •

edited

Loading

stipsan commented Feb 16, 2018

gdmitchell commented Feb 16, 2018

stipsan commented Feb 16, 2018 •

edited

Loading

gdmitchell commented Feb 16, 2018 •

edited

Loading

stipsan commented Feb 17, 2018

gdmitchell commented Feb 19, 2018

github-actions bot commented May 12, 2021

Fix race condition with --coverage and babel-jest identical file contents edge case #4432

Fix race condition with --coverage and babel-jest identical file contents edge case #4432

Conversation

stipsan commented Sep 5, 2017

cpojer Sep 5, 2017

Choose a reason for hiding this comment

stipsan Sep 5, 2017

Choose a reason for hiding this comment

stipsan Sep 5, 2017

Choose a reason for hiding this comment

cpojer Sep 5, 2017

Choose a reason for hiding this comment

stipsan Sep 5, 2017 • edited Loading

Choose a reason for hiding this comment

stipsan Sep 5, 2017

Choose a reason for hiding this comment

stipsan commented Sep 5, 2017

codecov-io commented Sep 6, 2017 • edited Loading

Codecov Report

cpojer commented Sep 6, 2017

cpojer commented Sep 6, 2017

stipsan commented Sep 6, 2017

gdmitchell commented Feb 10, 2018 • edited Loading

stipsan commented Feb 16, 2018

gdmitchell commented Feb 16, 2018

stipsan commented Feb 16, 2018 • edited Loading

gdmitchell commented Feb 16, 2018 • edited Loading

stipsan commented Feb 17, 2018

gdmitchell commented Feb 19, 2018

github-actions bot commented May 12, 2021

stipsan Sep 5, 2017 •

edited

Loading

codecov-io commented Sep 6, 2017 •

edited

Loading

gdmitchell commented Feb 10, 2018 •

edited

Loading

stipsan commented Feb 16, 2018 •

edited

Loading

gdmitchell commented Feb 16, 2018 •

edited

Loading